-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[8팀 최준만] [Chapter 1-2] 프레임워크 없이 SPA 만들기 #2
base: main
Are you sure you want to change the base?
Conversation
* 재귀적 평탄화 유틸 함수 구현 * 헬퍼 함수로 0을 제외 한 값은 falsy 체크를 통한 삭제
* 0을 제외한 falsy값 찾는 함수 유틸화 * 정규화 종료 단계에서 props 합쳐주기 로직 구현(이부분 조언 필요)
* input vNode가 배열일 경우, 재귀 실행 * 스트링, falsy등 처리 끝나면 dom 요소 생성
* createElement시 on~ 으로 시작하는 prop의 경우 addEvent처리 * WeakMap 자료형을 사용하여 등록될 이벤트 리스트 생성
* 업데이트일 수 도 있으니, 이벤트 전부 제거하고 시작 * container를 비우고 새로 append하도록 구현
* todo : 속성 교체 로직 구현 * MuataionObserver를 사용해 element 옵저빙 및 childList 제거 동작 포착 시 oldVNode 초기화
…어씌워지는 이슈가 있음 - 현재노드가 업데이트 되도록 수정
src/lib/elementObserver.js
Outdated
export function createElementChildRemoveObserver(elements, helper) { | ||
const observer = new MutationObserver((mutations) => { | ||
mutations.forEach((mutation) => { | ||
if (mutation.type === "childList") { | ||
mutation.removedNodes.forEach((node) => { | ||
console.log(node, "removed"); | ||
helper(node); | ||
}); | ||
} | ||
}); | ||
}); | ||
|
||
for (const element of elements) { | ||
observer.observe(element, { childList: true, subtree: false }); | ||
} | ||
|
||
return observer; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 함수는 더이상 사용되지 않는 함수인가요??
처음 들었을때 아이디어는 매우 좋다고 생각했는데 아쉽네요 ㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
디버깅하면서 없앴는데 다시한번 붙여봐야겠네요 ㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 다시 붙여보려 했는데, 사실 상위 엘리먼트에서 요소를 지운다 하더라도 oldVNode를 무조건 초기화 하는 것은 좋지 않을 것 같아요.
가령 layout이 같은 두개의 다른 페이지(프로필, 홈)를 오갈때, updateElement가 아닌 createElement를 시키는 것이 더 비효율적일 테니까요!
MutationObserver 자체는 충분히 사용 방안에 대해서 공부해볼만한 좋은 WebAPI 인 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
덕분에 최적한 방안에 대해서 생각해 볼 수 있어 좋았습니다. 감사해요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
준만님 부족한 실력으로 리뷰 남겨두었으니 한번 확인해보시면 좋을 것 같습니다 ㅎㅎ 전반적으로 자바스크립트의 고차 함수나 순수 함수와 같은 함수 관련된 로직을 너무 깔끔하게 잘 분리하시는 것 같아 �많이 배웠습니다 👍👍
export function checkNullishExceptZero(value) { | ||
// 0은 falsy한 값이 아니고 숫자로 처리할 것이다. | ||
if (value === 0) return true; | ||
return Boolean(value); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
자주 활용되는 로직을 유틸 함수로 별도 처리한 게 효율적이고 함수명도 직관적이라 가독성에도 좋은 것 같습니다 👍👍
이 함수 부분만 아래 함수와 동일하게 주석 컨벤션 맞추면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 0일때는 true
를 리턴하는 구문이 앞에 있으니까 `value === null || typeof value === "undefined" ..."로 0을 제외한 falsey한 값을 일일히 작성할 필요가 없겠네요!👍
time: Date.now(), | ||
content: document.getElementById("post-content").value, | ||
likeUsers: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document.getElementById로 ID를 하드코딩하는 것보다 event를 prop으로 받아서 event.target으로 접근하는 방법이 더 안정적일 것 같은데 어떻게 생각히사는지요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤식으로 해야할지 살짝 감이 오지 않습니다.
참고할 커밋 있을까요?
내일 같이 코드리뷰 시간에 얘기해보면 좋을 것 같아요!
src/components/posts/PostForm.jsx
Outdated
globalStore.setState({ | ||
posts: [ | ||
{ | ||
id: posts[0].id + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금은 posts가 기본 더미 데이터로 설정되어있긴한데 posts가 비어있다면? 이라는 생각이 들었어요 (저도 사실 고민하다가 Date.now()로 넣어놓고 따로 수정은 안했네요.. ㅎㅎ 더 좋은 방법 있는지 고민해봐도 좋을 것 같습니다)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id: posts ? posts.length + 1 : 1,
요래 바꿔봤는데 어떤가요? @devJayve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 store쪽 보시면 id값을 내림차순으로 변경해 놨습니다.
가장 최근에 들어온게 가장 아이디가 크도록요!
저희 과제에선 삭제가 없어서 이렇게 처리했습니다 ㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tc와 함께 구현해봤습니다.
src/lib/eventManager.js
Outdated
globalEvents = {}; | ||
} | ||
|
||
window.__myEventListeners = globalEvents; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 이벤트 처리가 전역을 대상으로 이루어지긴하는데 root 컨테이너 하위에서만 이벤트가 발생하니 setupEventListeners 내부에서 root 컨테이너에 대하여 설정해주는 건 어떠신가요 ??
export function setupEventListeners(root) {
root.__myEventListeners = globalEvents; // 예시
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 해먼드 코치님이 브라우저 콘솔에서 window.~ 으로 바로 확인해서 디버깅 하시는 것 보고 유용하다 싶어 썼고 삭제 해야할 코드입니다.
전역으로 접근 가능하게 하면 사실 매우매우 위험할 것 같네요.
확인 감사드려요~!
src/lib/eventManager.js
Outdated
*/ | ||
export function setupEventListeners(root) { | ||
for (const eventType in globalEvents) { | ||
root.removeEventListener(eventType, handleGlobalEvents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 updateAttributes
에서도 removeEvent
를 해주는데 root
에서 한 번 더 삭제해주는 이유가 있을까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 진짜 좋은 피드백인것 같습니다.
제 설계의도는 속성 업데이트 시 명령적으로 제거하는 것이 맞습니다.
위 코드는 이전에 근백님과 함께 디버깅 하면서 근백님의 코드 스타일을 제가 고려 없이 그대로 반영하다가 들어간 코드입니다.
예리한 피드백 감사합니다!
addEvent($el, prop.slice(2).toLowerCase(), props[prop]); | ||
continue; | ||
} | ||
$el.setAttribute(replaceIfPropIsClass(prop), props[prop]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로는 replacePropIsClass
로 별도의 함수로 뺐다면, 윗줄에서 한 번 실행하고 반환값을 받아서 prop
을 인자로 넘겨주는 쪽이 가독성이 더 좋을 것 같아요!
export function checkNullishExceptZero(value) { | ||
// 0은 falsy한 값이 아니고 숫자로 처리할 것이다. | ||
if (value === 0) return true; | ||
return Boolean(value); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 0일때는 true
를 리턴하는 구문이 앞에 있으니까 `value === null || typeof value === "undefined" ..."로 0을 제외한 falsey한 값을 일일히 작성할 필요가 없겠네요!👍
* 중복된 로직 변수화 * 전역에 globalEvents 객체를 할당하는 코드 제거 * 불필요한 조건문 네스팅 간소화
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 준만님 ~! 담당 학습메이트 오소현입니다 ㅎㅎ
준만님 요번주에 PR 구체적으로 작성해주시고, 구현한 함수들에 대한 개념도 굉장히 잘 작성해주셔서 제가 코드를 이해하면서 정말 많은 도움이 되었습니다 :) 신경써서 PR 작성해주셔서 너무 감사드려요~! 앞으로도 기대하겠습니다 ㅎㅎ
그리고 준만님께서 따로 구현한 함수에 대해 따로 테스트코드를 추가하신것 엄청 좋네요 ㅎㅎ 뭔가 테스트 코드 처음 짜보시기 어렵지 않으셨나요? 함수 제대로 불러와서 테스트 초기화부터 기댓값까지 엄청 잘 작성해주신 것 같습니다 :) 정말 너무 잘해주셨어요 ><
요번주에는 도운님 근백님께서 굉장히 리뷰를 잘 남겨주셔서 저도 팀원분들의 리뷰를보면서 같이 고민한 점도 있고, 제가 직접 실행하면서 더 제안해보면 좋을 점을 살포시 작성해보았습니다 ^0^ 부족하지만 제 리뷰가 준만님께서 이번주 과제를 다시한번 더 되돌아 볼 수 있는 좋은 기회가 되었으면 좋겠습니다 🫶🏻🫶🏻
크리스마스에도 all day,,,,과제를 열심히 해주셔서 너무 감사했어요 :) 쉬는 날없이 달리는 준만님,,! 최고!!
이번주 과제도 너무 고생 많으셨고, 다음주 과제도 화이팅입니다 아자자~!
------------------24.12.25 02:14--------------------- | ||
|
||
basic 테스트 코드 통과 | ||
챕터 1-1 테스트 코드 통과 | ||
|
||
------------------24.12.26 03:00--------------------- | ||
|
||
advanced 테스트 코드 통과 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해결하신 시간이랑 내용을 리드미에 작성해주셨네요?! 혹시 시간을 나눠서 작성하신 이유가 있으신가요😲😲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타임라인처럼 보고 해결에 며칠 걸렸는지 보기도 좋을것 같아서 그랬는데, 큰 의미는 없습니다 ㅎㅎ
src/components/posts/PostForm.jsx
Outdated
const onClickAddPost = () => { | ||
globalStore.setState({ | ||
posts: [ | ||
{ | ||
id: posts[0].id + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굿 저도 비슷한 생각을 가지고 있는데요!
저는 그래서 위의 코드에서 posts가 비어있을 때 에러가 발생할 수 있을때를 고려한 새 게시물의 id를 추가하는 id를 다음과 같이 추가로 구현해보면 어떨까 싶어요! 제가 지금 코드를 보고 생각한 코드는 다음과 같은데, 추가로 더 고려해보시면 좋을 것 같습니다!
const onClickAddPost = () => { | |
globalStore.setState({ | |
posts: [ | |
{ | |
id: posts[0].id + 1, | |
const onClickAddPost = () => { | |
const newId = posts.length > 0 ? Math.max(...posts.map(post => post.id)) + 1 : 1; | |
globalStore.setState({ | |
posts: [ | |
{ | |
id: newId, |
제가 보고 구현해본 내용은 다음과 같은데 테스트 코드는 통과하지만 개인적으로 마음에는 안드는 구조예요 ㅎㅎ,, 준만님께서 더 고민해보시면 좋을 것 같습니다:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정말 비슷한 코드를 원정님 커밋에서 봤는데, 제가 거기에 대안으로 코드를 작성해봤어요. 한번 보시고 이것도 리뷰해 주시죵 ㅎㅎ
id: posts.reduce((acc,cur)=> {
if(acc < cur) return cur;
return acc;
},0) + 1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[] 빈배열이 들어가도 1이 나오고, 가장 큰 값에 +1을 더하는 것은 동일합니다.
function updateAttributes($el, props) { | ||
for (const prop in props) { | ||
if (prop?.startsWith("on")) { | ||
addEvent($el, prop.slice(2).toLowerCase(), props[prop]); | ||
continue; | ||
} | ||
$el.setAttribute(replaceIfPropIsClass(prop), props[prop]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateAttributes 함수를 보면 기존 코드는 모든 속성을 매번 설정하고 있어서 DOM 요소에 이미 설정된 속성이라도 변경 없이도 불필요하게 다시 설정되고 있네요 혹시 맞나용??
function updateAttributes($el, props) { | |
for (const prop in props) { | |
if (prop?.startsWith("on")) { | |
addEvent($el, prop.slice(2).toLowerCase(), props[prop]); | |
continue; | |
} | |
$el.setAttribute(replaceIfPropIsClass(prop), props[prop]); | |
} | |
} | |
function updateAttributes($el, props, prevProps = {}) { | |
for (const key in prevProps) { | |
if (!(key in props)) { | |
$el.removeAttribute(key); | |
} | |
} | |
for (const key in props) { | |
if (props[key] !== prevProps[key]) { | |
if (key.startsWith('on')) { | |
addEvent($el, key.slice(2).toLowerCase(), props[key]); | |
} else if (key === 'className') { | |
$el.setAttribute('class', props[key]); | |
} else { | |
$el.setAttribute(key, props[key]); | |
} | |
} | |
} | |
} |
제가 개선해보면서 addEvent 함수를 호출하기 전에 이벤트 리스너 속성이 변경되었는지 확인하는 로직까지 한번 추가 해봤는데 요렇게 추가로 해보시는건 어떤가요?? 조금 더 고도화 하면 좋을 것 같습니다 :) 이 함수의 역할이 잘 보이게 간결하게 잘 작성하신 것 같아요!! 좋습니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 코드 개선까지 감사합니다.
여기서 궁금한게,
props[key] !== prevProps[key]
코드에서 핸들러 함수가 상이한 것을 체크할 수 있나요??
src/lib/elementObserver.js
Outdated
export function createElementChildRemoveObserver(elements, helper) { | ||
const observer = new MutationObserver((mutations) => { | ||
mutations.forEach((mutation) => { | ||
if (mutation.type === "childList") { | ||
mutation.removedNodes.forEach((node) => { | ||
console.log(node, "removed"); | ||
helper(node); | ||
}); | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번 과제에서는 따로 사용되지 않아보이는데 코드를 읽어보면 도입하시려는 목적이 DOM 요소들의 자식 요소가 제거되고 나서, 각 제거된 요소에 대해 따로 이벤트 처리를 하시려고 한 것 같네요 어떤 목적으로 구현하려고 하셨나요??? 궁금합니다👀👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아~ MutationObserver를 통해서 최상위 엘리먼트 root 혹은 body등에서 삭제되는 순간 oldVNode를 초기화 해 주기 위해 넣은 코드입니다. 하지만 코드 개발 과정에서 초기화 과정이 불필요하다고 판단하여(오히려 성능 저하시킴)
뺴게 되었습니다!!
초기에 넣게된 이유는 테스트코드에서 before,afterEach등의 동작에서 자꾸 dom을 초기화 하는데 이부분 때문에 이슈가 난다고 판단해서 해당 타이밍을 포착해 다 초기화 해주려고 한 것이었습니다.
src/lib/eventManager.js
Outdated
export function setupEventListeners(root) { | ||
for (const eventType in globalEvents) { | ||
root.removeEventListener(eventType, handleGlobalEvents); | ||
root.addEventListener(eventType, handleGlobalEvents); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 @chb6734 분의 코드를 함께 보다가 setupEventListeners 함수에 필요할 때만 이벤트 리스너를 등록하도록 처음 등록할때만 이벤트를 추가하도록 하는것도 좋을 것 같아요~!
현재 코드에서는 이벤트 리스너를 제거한 다음 다시 추가하는 로직을 구현하고 있는데 처음 등록할때에만 추가하는 것도 좋을 것 같습니다
export function setupEventListeners(root) { | |
for (const eventType in globalEvents) { | |
root.removeEventListener(eventType, handleGlobalEvents); | |
root.addEventListener(eventType, handleGlobalEvents); | |
} | |
} | |
export function setupEventListeners(root) { | |
for (const eventType in globalEvents) { | |
if (!root.getAttribute(`data-listener-${eventType}`)) { | |
root.addEventListener(eventType, handleGlobalEvents); | |
root.setAttribute(`data-listener-${eventType}`, 'true'); | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 필요할때만 초기화 하는 좋은 코드네요
참고해서 구현해보겠습니다!!
/** | ||
* @description 이벤트 프로퍼티인 경우 on 제거하고 소문자로 변경 | ||
* @param {string} prop | ||
* @returns {string} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
js doc 잘 활용해 주셔서 가독성이 올라갔어요~!~! 너무 잘하셨습니다 ㅎㅎ 저는 주로 메인 로직 함수에만 잘 작성해주는 편인데, 준만님의 주석을 작성해주시는 기준이 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감삼당
저는 네이밍 만으로는 설명이안되거나,
vscode에서 js의 경우 jsdoc에 작성된 type에 맞춰 자동완성을 할 수 있게 보조해 주는 기능이 있어서 씁니다.
HTMLElement의 내부 속성과 메서드가 참 많잖아요?
자동완성 없이 쓰면 어떤게 있는지 아주 자주쓰는 것을 제외하고는 알수가 없더라구요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function clearEvents() { | ||
globalEvents = {}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 함수의 목적이 전역 globalEvents 객체를 완전히 초기화 하는 역할을 하고 있는 것 같아요!
이렇게 globalEvents = {}; 구현해주셔도 좋은데 개별 이벤트 유형에 대해 이벤트 리스너를 제거하도록 각각 세부적으로 구현해보는 것도 좋은 것 같아요 :)
export function clearEvents() { | |
globalEvents = {}; | |
} | |
export function clearEvents() { | |
for (const eventType in globalEvents) { | |
const eventMap = globalEvents[eventType]; | |
eventMap.forEach((_, element) => { | |
if (element && element.removeEventListener) { | |
element.removeEventListener(eventType, handleGlobalEvents); | |
} | |
}); | |
delete globalEvents[eventType]; | |
} | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 직접 순회하면서 지워주면 어떤 이점이 있을까요??
export function recursiveFlatten(arr, helper = () => true) { | ||
return arr.reduce((acc, cur) => { | ||
if (Array.isArray(cur)) { | ||
const flattenedChild = recursiveFlatten(cur, helper); | ||
acc = acc.concat(flattenedChild); | ||
} else { | ||
if (helper(cur)) acc.push(cur); | ||
} | ||
return acc; | ||
}, []); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배열 병합하실때 concat 메소드를 사용하시는 군요,,
concat 이랑 push를 둘다 사용하셨는데 저는 둘다 push로 한번 구현해봤습니다 ㅎㅎ
export function recursiveFlatten(arr, helper = () => true) { | |
return arr.reduce((acc, cur) => { | |
if (Array.isArray(cur)) { | |
const flattenedChild = recursiveFlatten(cur, helper); | |
acc = acc.concat(flattenedChild); | |
} else { | |
if (helper(cur)) acc.push(cur); | |
} | |
return acc; | |
}, []); | |
} | |
export function recursiveFlatten(arr, helper = () => true) { | |
return arr.reduce((acc, cur) => { | |
if (Array.isArray(cur)) { | |
const flattenedChild = recursiveFlatten(cur, helper); | |
acc.push(...flattenedChild); // spread 연산자를 사용하여 배열 병합을 최적화 | |
} else if (helper(cur)) { | |
acc.push(cur); | |
} | |
return acc; | |
}, []); | |
} | |
concat ()을 자주 사용하시나용??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
push를 쓰게되면 스프레드 해주어야하고 이과정에서. 불필요한 메모리가 낭비되지 않을까~ 라고 생각했습니당!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventManager.js
를 참고하려 왔다가 준만님의 노력과 고민이 가득하신 코드를 보고 많이 반성했습니다🥲 정말 많이 고민하신게 느껴져요!
특히 jsdoc이나 테스트 코드등 과제에서 놓치기 쉬운부분까지 신경쓰신거 대단하십니다👍👍
제가 보면서 궁금한점 몇개 코멘트로 달아두었습니다 많이 배우고 갑니다!
} | ||
} | ||
|
||
export function handleGlobalEvents(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventManager에서 중복된 이벤트 이슈를 어떻게 해결해야할지 감이 안잡혔는데, 준만님 코드를 보고 많이 배웠습니다!👍👍👍
@@ -20,7 +22,8 @@ export const Post = ({ | |||
<p>{content}</p> | |||
<div className="mt-2 flex justify-between text-gray-500"> | |||
<span | |||
className={`like-button cursor-pointer${activationLike ? " text-blue-500" : ""}`} | |||
className={`like-button cursor-pointer${likeUsers.includes(currentUser?.username) ? " text-blue-500" : ""}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 제시된 activationLike
를 그대로 사용했는데, 어차피 likeUsers를 전달해주니, 굳이 위에서 체크할 필요가 없었군요..
꼼꼼하게 체크하신게 느껴집니다 👍
import { addEvent } from "./eventManager"; | ||
|
||
export function createElement(vNode) {} | ||
export function createElement(vNode) { | ||
if (typeof vNode === "function" || typeof vNode?.type === "function") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
normalizeVNode.js
의 vNode.type === "function"
를 통해
컴포넌트 함수는 다 처리된 후 createElement로 전달될 것 같은데 혹시 한번 더 체크하시는 이유가 있으신지, 예외케이스가 있으셨는지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오~ 예리하십니다.
저희가 정규화 단계에서 모든 함수 호출을 하기때문에 해당 조건문에 걸릴 일이 없을 것 같네요.
예리한 피드백 감사합니다.
if (Array.isArray(vNode)) { | ||
const frag = document.createDocumentFragment(); | ||
vNode.forEach((node) => { | ||
frag.append(makeElement(node)); | ||
}); | ||
return frag; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 vNode가 배열일 경우 createElement
가 아니라 makeElement
로 처리해주셨는데, 배열 요소에 vNode === "string" || vNode === "boolean"
요런 케이스에 해당하는 요소가 올 경우에 오류가 발생하지 않으셨는지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
진짜 문제의 소지가 있겠네요!!
리뷰 감사합니다. 고쳐야될 코드같아요!!
과제 체크포인트
기본과제
가상돔을 기반으로 렌더링하기
클릭해서 열기
이벤트 위임
심화 과제
1) Diff 알고리즘 구현
2) 포스트 추가/좋아요 기능 구현
과제 셀프회고
이번 주 과제는 tc가 잘 나뉘어져 있어서 스텝 바이 스텝으로 접근이 쉬웠습니다.
다만, 재귀적 동작 수행과, 속성 할당에 있어서 예외 케이스에 대한 충분한 고찰이 참 많이 필요했다고 느낍니다.
엣지 케이스를 사전에 파악하는 방법이 뭐가 있을지 생각해 보았고, 더 치밀한 tc로 해결할 수 있지 않을까 생각합니다.
이부분을 남은 시간동안 구현해보려 합니다.
기술적 성장
학습 효과 분석
과제 회고
구현과정
코드의 흐름
1. VNode(Virtual DOM) 구현
1.1 createVNode
1.2 normalizeVNode
1.3 createElement
1.4 render
1.5 setupEventListeners
먼저, setup을 해주기 이전에, createElement에서 이벤트 속성을 넣어주는 부분을 살펴보자(1.5.1~1.5.2)
1.5.1 updateAttributes
1.5.2 addEvent
1.5.3 setupEventListeners
종결(?)
여기까지가 최초 렌더링을 위한 VNode 생성과 실제 DOM 변환, 이벤트 위임까지의 과정이다.
이후에는 상태 변경에 따른 렌더링을 위한 diff 알고리즘을 구현하고, 이를 통해 변경된 부분만 다시 렌더링하는 과정을 서술한다.
diff가 없이 전부 repaint하는 방식을 짤막하게 보여주며 마무리한다.
클릭해서 열기
2. diff 알고리즘 구현
2.0 최초 렌더링 후 oldVNode 저장
2.1 updateElement
2.1.1 oldVNode 존재 여부에 따른 분기 처리
2.1.2 updateElement
2.1.3 updateAttributes
2.2 render 와 eventListener
2.3 종결!
##마치며
가상돔을 이용해 렌더링 과정을 구현하면서 하나의 문장으로 정리하자면,
** 비교는 가상돔끼리, 표현은 실제 돔에! ** 라고 할 수 있겠다.
가상돔끼리의 비교를 통해 실제 돔과 비교보다 훨씬 가볍게 변경된 부분을 포착하고 업데이트할 수 있었다.
선언적으로 저장된 함수를 실행시켜 가상돔을 만들고, 이를 실제 돔으로 변환하여 렌더링하는 과정을 통해 리액트의 동작 원리에 대한 이해를 높일 수 있었다.
리뷰 받고 싶은 내용